refactor: move artist detail reads to api#1656
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 4 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR removes a local Changes
Sequence DiagramsequenceDiagram
participant Component as useArtistInstruction/<br/>useArtistKnowledge
participant Privy as Privy Auth
participant Helper as getArtist Helper
participant ClientAPI as Client API<br/>/api/artists/{id}
Component->>Privy: getAccessToken()
Privy-->>Component: accessToken
Component->>Helper: getArtist(artistId, accessToken)
Helper->>ClientAPI: GET /api/artists/{artistId}<br/>Authorization: Bearer token
ClientAPI-->>Helper: ArtistRecord
Helper-->>Component: artist (typed)
Component->>Component: Extract instruction/<br/>knowledges from artist
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8dd9dfdc0
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
hooks/useArtistInstruction.ts
Outdated
| enabled: Boolean(artistId), | ||
| queryFn: async () => { | ||
| if (!artistId) return undefined; | ||
| const res = await fetch(`/api/artist?artistId=${encodeURIComponent(artistId)}`); | ||
| if (!res.ok) return undefined; | ||
| const json = await res.json(); | ||
| return json?.artist?.instruction || undefined; | ||
| const accessToken = await getAccessToken(); | ||
| if (!accessToken) return undefined; |
There was a problem hiding this comment.
Gate artist instruction query on authentication state
This query is enabled only by artistId, so if getAccessToken() returns null during Privy initialization it returns undefined as a successful result and caches that value for 5 minutes (staleTime), which prevents an immediate refetch once auth becomes ready because the query key does not include auth state. In that scenario, instruction/knowledge can remain blank until invalidation or staleness; the same pattern is repeated in useArtistKnowledge, so these queries should be gated on authentication readiness (or treat missing token as an error/retry path) instead of returning cached empty data.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
hooks/useArtistInstruction.ts (1)
13-17: Consider extracting shared authenticated artist-read logic.The token-fetch +
getArtistflow is duplicated withhooks/useArtistKnowledge.ts. A tiny shared helper hook would keep behavior consistent and reduce drift.As per coding guidelines, "DRY: Extract common hook logic into shared utilities".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/useArtistInstruction.ts` around lines 13 - 17, Extract the duplicated token-fetch + artist retrieval logic from useArtistInstruction.ts and useArtistKnowledge.ts into a shared helper (e.g., fetchAuthenticatedArtist or useAuthenticatedArtist) that calls getAccessToken() and, if present, calls getArtist(artistId, accessToken) and returns the artist or undefined; then update both hooks to call that helper instead of duplicating the getAccessToken/getArtist sequence so behavior stays consistent and reduces drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hooks/useArtistInstruction.ts`:
- Around line 9-15: The query currently uses queryKey: ["artist-instruction",
artistId] and can cache a failing unauthenticated result from getAccessToken();
update useArtistInstruction to include the auth state from usePrivy() (usePrivy
-> getAccessToken and authenticated) in the query identity and gating: add
authenticated to the queryKey (e.g., ["artist-instruction", artistId,
authenticated]) and to enabled (enabled: Boolean(artistId) && authenticated) so
the query won't cache/return undefined pre-auth, or alternatively ensure the
hook invalidates the ["artist-instruction", artistId] query when authenticated
changes.
In `@hooks/useArtistKnowledge.ts`:
- Around line 10-16: The query's enabled flag only checks artistId but the
queryFn calls getAccessToken(), causing empty results to be cached if auth isn't
ready; update the hook that defines queryKey / enabled (the React Query call
with queryKey: ["artist-knowledge", artistId] and enabled: Boolean(artistId)) to
also consider auth readiness by importing and using authenticated from
usePrivy() — either add authenticated to the enabled condition (e.g., enabled:
Boolean(artistId) && authenticated) or include authenticated in the queryKey
(e.g., ["artist-knowledge", artistId, authenticated]) so the query won't run or
will refetch only when auth state is ready and cached results are invalidated
when auth changes.
In `@lib/getArtist.tsx`:
- Around line 18-27: The getArtist function currently swallows all errors (both
network and non-OK responses) by returning null, preventing React Query (used by
useArtistKnowledge and useArtistInstruction) from handling retries and error
states; replace the silent failure behavior by removing the broad try/catch (or
rethrowing inside it) and, when response.ok is false, throw a descriptive Error
(include status and any relevant response text) instead of returning null so
callers receive thrown errors and React Query can handle them properly.
---
Nitpick comments:
In `@hooks/useArtistInstruction.ts`:
- Around line 13-17: Extract the duplicated token-fetch + artist retrieval logic
from useArtistInstruction.ts and useArtistKnowledge.ts into a shared helper
(e.g., fetchAuthenticatedArtist or useAuthenticatedArtist) that calls
getAccessToken() and, if present, calls getArtist(artistId, accessToken) and
returns the artist or undefined; then update both hooks to call that helper
instead of duplicating the getAccessToken/getArtist sequence so behavior stays
consistent and reduces drift.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5f1bfeac-48cd-4e79-a8fa-f09b878c1b2e
📒 Files selected for processing (4)
app/api/artist/route.tsxhooks/useArtistInstruction.tshooks/useArtistKnowledge.tslib/getArtist.tsx
💤 Files with no reviewable changes (1)
- app/api/artist/route.tsx
There was a problem hiding this comment.
1 issue found across 4 files
Confidence score: 3/5
- There is a concrete regression risk in
lib/getArtist.tsx: suppressing errors and returningnullin theuseQueryqueryFnpath can mark failed fetches as successful results. - This is why the score is moderate (not low): the issue is high-confidence and user-facing, since React Query error/loading behavior may not trigger correctly when failures are swallowed.
- Pay close attention to
lib/getArtist.tsx- ensure failures are thrown (not converted tonull) so React Query can enter its error state correctly.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="lib/getArtist.tsx">
<violation number="1" location="lib/getArtist.tsx:18">
P1: Error suppression here prevents React Query from handling failures correctly. Both callers use `getArtist` inside `useQuery` `queryFn`, which treats any returned value (including `null`) as a successful result. This silently swallows network errors and non-OK responses, disabling retries, error states, and error boundaries.
Throw on failure instead so React Query can manage the error lifecycle:
```ts
if (!response.ok) {
throw new Error(`Failed to fetch artist: ${response.status}`);
}
And remove the outer try-catch so fetch errors also propagate.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review, or [fix all with cubic](https://www.cubic.dev/action/fix/pr/recoupable/chat/1656/ai_pr_review_1775688164042_ee849120-c9a7-4edf-b7b9-b6643714a4ea?entrySource=github_ui_to_cubic_ui).</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/useArtistKnowledge.ts">
<violation number="1" location="hooks/useArtistKnowledge.ts:10">
P2: Including `authenticated` in `queryKey` is a React Query anti-pattern when it's already used in `enabled`. This creates separate cache entries per auth state, so toggling auth (e.g., token refresh) discards cached data and forces unnecessary refetches. Remove it from the key and rely on `enabled` alone to gate execution.</violation>
</file>
<file name="hooks/useArtistInstruction.ts">
<violation number="1" location="hooks/useArtistInstruction.ts:9">
P3: Including `authenticated` in the query key is unnecessary since it's already used in `enabled` to gate execution. It creates an empty `[..., false]` cache entry that's never populated and separates the cache from the `[..., true]` entry. Use `enabled` alone for gating.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Requires human review: This PR refactors core data fetching logic and introduces mandatory authentication (Privy tokens) for artist details, which is an architectural and security change requiring human review.
|
Closing this PR because the migrated single-artist detail read path does not currently have a live in-app consumer. The local route removal and dead-hook migration should not move forward until a real mounted caller for |
Summary
Verification
Summary by cubic
Move artist detail reads to the authenticated API via
lib/getArtist, removing the local route. This secures access with Privy tokens, improves error handling, and simplifies React Query keys.lib/getArtist, calling${getClientApiBaseUrl()}/api/artists/:idwith aBearertoken; returnsArtistRecord | nulland throws on non-OK.useArtistInstructionanduseArtistKnowledgeto usegetArtist; use@privy-io/react-authgetAccessToken, enable only when signed in, throw if no token; simplified query keys to["artist-instruction", artistId]and["artist-knowledge", artistId].artist.knowledges. Removedapp/api/artist/route.tsx.Written for commit 1eb00d9. Summary will update on new commits.
Summary by CodeRabbit